Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCS_MAVLINK: Fix Deprecation of MISSION_REQUEST #28854

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

menschel
Copy link
Contributor

Minor Protocol Issue; Finding during another investigation mavlink/qgroundcontrol#12184

MISSION_REQUEST is deprecated since 2020, replaced with MISSION_REQUEST_INT, but ardupilot still uses it to request mission items from GCS.

https://mavlink.io/en/services/mission.html#uploading_mission
upload

wireshark_mavlink_upload

@menschel menschel force-pushed the pr-fix-deprecated-mavlink-mission-request branch from b86d074 to dd04ec6 Compare December 13, 2024 14:51
@menschel
Copy link
Contributor Author

menschel commented Dec 13, 2024

After applying the patch.

Mavlink_Mission_Upload_with_Fix

@menschel menschel force-pushed the pr-fix-deprecated-mavlink-mission-request branch from dd04ec6 to 1c43fae Compare December 13, 2024 15:39
@peterbarker
Copy link
Contributor

We continue to request MISSION_ITEM so as not to break older ground control stations.

I deal with one which this PR would adversely affect (I'm nudging the author to update....).

Which GCSs have you tested with these changes?

@menschel
Copy link
Contributor Author

menschel commented Dec 14, 2024

I deal with one which this PR would adversely affect (I'm nudging the author to update....).

Please add a link there that marks this PR as duplicate, so it is to be closed along with it.

Which GCSs have you tested with these changes?

I'm hunting some Mavlink issues with QGroundControl and ELRS 900M, very frustrating topic because ELRS 2G4 works perfectly and mLRS 900M also works perfectly.

EDIT:
These also work with the change.

@menschel
Copy link
Contributor Author

menschel commented Dec 14, 2024

Tested again after a git hard reset onto the change, and the internal error is gone. It was likely the check in line 371 being reverted on stashed changes.

@peterbarker
Copy link
Contributor

Requires ArduPilot/MAVProxy#1492

@peterbarker
Copy link
Contributor

I deal with one which this PR would adversely affect (I'm nudging the author to update....).

Please add a link there that marks this PR as duplicate, so it is to be closed along with it.

That particular GCS is not open-source.

Which GCSs have you tested with these changes?

I'm hunting some Mavlink issues with QGroundControl and ELRS 900M, very frustrating topic because ELRS 2G4 works perfectly and mLRS 900M also works perfectly.

EDIT: These also work with the change.

* [APM-Planner_2-2.0.30-rc3](https://github.com/ArduPilot/apm_planner/releases/tag/2.0.30-rc3)

* [MissionPlanner1.3.82](https://github.com/ArduPilot/MissionPlanner/releases/tag/MissionPlanner1.3.82)

Nice! I've fixed MAVProxy to work with it.

We'll need to follow a process here so as to give people plenty of warning that things are going to break. It is well past time that we did follow some process to get this done.

There are many GCSs and tools out there that would be adversely affected by this patch if we were to simply merge it. MAVProxy users are likely to update relatively frequently - but uGCS users perhaps not.

We may need to carry an options bit for a while to allow a user to get the old behaviour back.

I am curious to know what real-world problem you are trying to solve with this patch. I assume there's a client which only handles MISSION_REQUEST_INT, and can't be modified to also handle MISSION_REQUESTs from the autopilot?

@menschel
Copy link
Contributor Author

We'll need to follow a process here so as to give people plenty of warning that things are going to break. It is well past time that we did follow some process to get this done.

It would be the professional way to introduce breaking changes with a new major version.
A deprecation warning in the release notes from 4.6 onward is a fair warning of an error that will hit with 5.0 .

I am curious to know what real-world problem you are trying to solve with this patch. I assume there's a client which only handles MISSION_REQUEST_INT, and can't be modified to also handle MISSION_REQUESTs from the autopilot?

None, just a side finding. A decade or two on the job, does teach to address technical debt immediately.

@peterbarker
Copy link
Contributor

We'll need to follow a process here so as to give people plenty of warning that things are going to break. It is well past time that we did follow some process to get this done.

It would be the professional way to introduce breaking changes with a new major version. A deprecation warning in the release notes from 4.6 onward is a fair warning of an error that will hit with 5.0 .

We don't tend to use the major-version-breaks-things semantics suggested by a lot of people.

Instead code tends to get made optionally-compilable. We send a warning message for a while (yes, the timekeeping on this is that exact...), then we compile the code out by default, then we remove the code. All in "approximate time". I've started to leave comments around this code indicating when things should happen, but typically these steps occur on minor version changes - which are supposed to be every 6 months or so.

I am curious to know what real-world problem you are trying to solve with this patch. I assume there's a client which only handles MISSION_REQUEST_INT, and can't be modified to also handle MISSION_REQUESTs from the autopilot?

None, just a side finding. A decade or two on the job, does teach to address technical debt immediately.

OK, so no immediate concern here. Expiring technical debt is rather easier when you control all of the moving parts :-)

I'll put a message out on the ardupilot-gcs mailing list to try to prompt people to ensure they support MISSION_ITEM_INT requests from the autopilot.

https://github.com/ardupilot/ardupilot/blob/master/libraries/GCS_MAVLink/GCS_config.h#L107 shows where we are warning on the other side - so if a GCS is requesting items using MISSION_REQUEST rather than MISSION_REQUEST_INT` they get a one-time warning.

I think we will probably need to do some form of probing for support here. So request an item using MISSION_REQUEST_INT and if no response is received fall back to MISSION_REQUEST - and bitch at the user.

That's probably stashing away the currently-sending-type (MISSION_REQUEST or MISSION_REQUEST_INT), and a pointer to the relevant method (mavlink_msg_mission_request_send) or mavlink_msg_mission_request_int_send). A small patch in MissionItemProtocol::updates timeout handling code can switch to the non-INT version and warn the user. MAVProxy can be used for testing by reverting that PR I merged against it.

Did you want to do that work (or something better!), or shall I?

@menschel
Copy link
Contributor Author

Did you want to do that work (or something better!), or shall I?

I will add once I'm done with the 900M ELRS Mavlink Issue.
This will eventually hit MissionPlanner as well.

@menschel menschel force-pushed the pr-fix-deprecated-mavlink-mission-request branch 2 times, most recently from 0345c0b to b5ddb99 Compare December 17, 2024 18:33
Use MISSION_REQUEST_INT to request mission items from
ground control station. Fallback to the deprecated
MISSION_REQUEST after the first timeout and if
the ground control station then answers, send a text warning
to be displayed in the ground station.
Some capabilities are available but not advertised.

FIXME: MAV_PROTOCOL_CAPABILITY_MISSION_FLOAT is deprecated!
@menschel menschel force-pushed the pr-fix-deprecated-mavlink-mission-request branch from b5ddb99 to eaee905 Compare December 17, 2024 20:22
@menschel
Copy link
Contributor Author

Well, tested, works but produces false positive warnings on high latency links.

I'm against this workaround and support old stuff forever approach.

I prefer the clean cut method. Drop support at next major version.

There is also an issue with the advertised capabilities and the mavlink fork of ardupilot not being up to date.

-2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants